Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Memory management improvements #508

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

PSI-Rockin
Copy link
Contributor

@PSI-Rockin PSI-Rockin commented May 3, 2024

The existing memory management system is somewhat dodgy, to put it lightly. This PR aims to rewrite it almost entirely to improve overall accuracy of the emulator. This is currently a work in progress and will require quite a bit of time to implement all the planned features.

What needs to be done next (in no particular order):

  • Move virtual memory management out of Memory to a KPageTable class to allow for multiple processes in the future
  • Map the configuration and shared memory pages properly
  • Add accurate MemoryState tracking
  • Readd 3dsx/ELF loading, which was disabled in the initial commit
  • Implement Free in ControlMemory
  • Implement Unmap in ControlMemory
  • Implement Protect in ControlMemory
  • Parse kernel capabilities in the exheader to map memory regions such as VRAM (this is currently hardcoded)
  • Reduce the size of TLS to 0x200 bytes per thread, rather than the current 0x1000
  • Add support for large memory applications (e.g. Sun and Moon, which requires 80 MB of application memory)
  • Make services responsible for creating shared memory blocks

PSI-Rockin added 10 commits May 2, 2024 21:40
Disables a lot of functionality... but I didn't want to commit too much to this commit
Also reworks virtual memory management somewhat (but needs more work)
Previously all non-free blocks were marked as Reserved
Can now use a single function to change either state, permissions, or both
Also merge vmem blocks that have the same state and permissions
Fix minor bug with permission tracking
Also do a sanity check to make sure the memory region is free for linear allocations
Not quite correct, but nothing to be done until process management is improved
Also remove the stack limit for CXIs (thanks amogus)
Not really correct, but it should be accurate for applications at least
@@ -265,7 +266,8 @@ void Kernel::getProcessInfo() {
// According to 3DBrew: Amount of private (code, data, heap) memory used by the process + total supervisor-mode
// stack size + page-rounded size of the external handle table
case 2:
regs[1] = mem.getUsedUserMem();
// FIXME
regs[1] = fcramManager.getUsedCount(FcramRegion::App);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO for future me before merging: Check if this needs * pageSize

@@ -342,7 +344,7 @@ void Kernel::getSystemInfo() {
switch (subtype) {
// Total used memory size in the APPLICATION memory region
case 1:
regs[1] = mem.getUsedUserMem();
regs[1] = fcramManager.getUsedCount(FcramRegion::App);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

u8* fcram;
u8* dspRam; // Provided to us by Audio
u8* vram; // Provided to the memory class by the GPU class

u64& cpuTicks; // Reference to the CPU tick counter
using SharedMemoryBlock = KernelMemoryTypes::SharedMemoryBlock;

// TODO: remove this reference when Peach's excellent page table code is moved to a better home
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's true

@@ -175,6 +175,7 @@ set(KERNEL_SOURCE_FILES src/core/kernel/kernel.cpp src/core/kernel/resource_limi
src/core/kernel/address_arbiter.cpp src/core/kernel/error.cpp
src/core/kernel/file_operations.cpp src/core/kernel/directory_operations.cpp
src/core/kernel/idle_thread.cpp src/core/kernel/timers.cpp
src/core/kernel/fcram.cpp
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fcram.hpp also needs to be added to the HEADER_FILES list otherwise Visual Studio's filters don't list it.


class Memory;

typedef std::list<FcramBlock> FcramBlockList;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using FcramBlockList = std::list

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file also could use a quick clang-format run

struct Region {
struct Block {
s32 pages;
s32 pageOffs;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend changing offs to offset

@@ -139,8 +161,8 @@ class Memory {
static constexpr u32 DSP_DATA_MEMORY_OFFSET = u32(256_KB);

private:
std::bitset<FCRAM_PAGE_COUNT> usedFCRAMPages;
std::optional<u32> findPaddr(u32 size);
//std::bitset<FCRAM_PAGE_COUNT> usedFCRAMPages;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be deleted since they're totally superseded

void KFcram::reset(size_t ramSize, size_t appSize, size_t sysSize, size_t baseSize) {
fcram = mem.getFCRAM();
refs = std::unique_ptr<u32>(new u32[ramSize >> 12]);
memset(refs.get(), 0, (ramSize >> 12) * sizeof(u32));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::memset

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file can also use a quick clang-format, seems like headers at the top at least are unformatted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants